http: provide keep-alive timeout response header #34561

ronag · 2020-07-30T14:59:56Z

In http 1.1 persistent connection protocol there is a timing race where the
client sends the request and then the server kills the connection
(due to inactivity) before receiving the client's request.

By providing a keep-alive header it is possible to provide the client a
hint of when idle timeout would occur and avoid the race.

Fixes: #34560

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

ronag · 2020-07-30T15:00:05Z

@nodejs/http

In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: nodejs#34560

ronag · 2020-07-30T15:28:52Z

lib/_http_outgoing.js

@@ -424,6 +427,10 @@ function _storeHeader(firstLine, headers) {
        (state.contLen || this.useChunkedEncodingByDefault || this.agent);
    if (shouldSendKeepAlive) {
      header += 'Connection: keep-alive\r\n';
+      if (this._keepAliveTimeout) {
+        const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;


Should we always set this to less than the actual timeout? And if so what is a good algorithm?

Based nodejs/undici#279 I feel that any client should always assume a value less than the hint. So further reducing it here can be counter productive.

ronag · 2020-07-30T15:58:09Z

@nodejs/http @nodejs/web-server-frameworks

nodejs-github-bot · 2020-07-30T15:58:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32556/

nodejs-github-bot · 2020-07-30T16:09:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32558/

mcollina

lgtm, it would be good if our own agent would do somethign about this as well.

ronag · 2020-08-01T13:05:23Z

Landed in 849d9e7

In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: #34560 PR-URL: #34561 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>

killagu · 2020-09-07T05:27:57Z

lib/_http_outgoing.js

@@ -424,6 +427,10 @@ function _storeHeader(firstLine, headers) {
        (state.contLen || this.useChunkedEncodingByDefault || this.agent);
    if (shouldSendKeepAlive) {
      header += 'Connection: keep-alive\r\n';
+      if (this._keepAliveTimeout) {
+        const timeoutSeconds = MathFloor(this._keepAliveTimeout) / 1000;
+        header += `Keep-Alive: timeout=${timeoutSeconds}\r\n`;


Not add the header if the Keep-Alive exits maybe better.

PR-URL: #35138 Fixes: #34561 Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

In http 1.1 persistent connection protocol there is a timing race where the client sends the request and then the server kills the connection (due to inactivity) before receiving the client's request. By providing a keep-alive header it is possible to provide the client a hint of when idle timeout would occur and avoid the race. Fixes: #34560 PR-URL: #34561 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Pranshu Srivastava <rexagod@gmail.com>

Backport-PR-URL: #35623 PR-URL: #35138 Fixes: #34561 Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

PR-URL: nodejs#35138 Fixes: nodejs#34561 Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com>

ronag requested a review from mcollina July 30, 2020 14:59

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 30, 2020

ronag force-pushed the keep-alive-hint branch from d2c88ca to 9719ec1 Compare July 30, 2020 15:00

addaleax approved these changes Jul 30, 2020

View reviewed changes

fixup

003e581

ronag force-pushed the keep-alive-hint branch from 686f8f2 to 003e581 Compare July 30, 2020 15:06

ronag commented Jul 30, 2020

View reviewed changes

fixup

b4e8b79

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2020

mcollina approved these changes Jul 30, 2020

View reviewed changes

lpinca approved these changes Jul 30, 2020

View reviewed changes

himself65 approved these changes Jul 30, 2020

View reviewed changes

rickyes approved these changes Jul 31, 2020

View reviewed changes

trivikr approved these changes Jul 31, 2020

View reviewed changes

rexagod approved these changes Jul 31, 2020

View reviewed changes

ronag closed this Aug 1, 2020

codebytere mentioned this pull request Aug 10, 2020

v14.8.0 proposal #34704

Merged

ronag mentioned this pull request Aug 17, 2020

Performance requests nodejs/llhttp#64

Closed

This was referenced Aug 25, 2020

Node.js 14.x integration tests are failing on response size asserts macbre/phantomas#769

Merged

Use nginx to make integration tests more deterministic macbre/phantomas#773

Merged

atian25 mentioned this pull request Sep 7, 2020

fix: only set keep-alive header before Node.js 14.8.0 eggjs/egg#4457

Merged

4 tasks

killagu reviewed Sep 7, 2020

View reviewed changes

This was referenced Sep 8, 2020

fix: only set keep-alive when not exists #35096

Closed

fix: only set keep-alive when not exists #35138

Closed

codebytere mentioned this pull request Sep 28, 2020

v12.19.0 proposal #35401

Merged

rickyes mentioned this pull request Nov 27, 2020

fix: skip specified Node.js version to set keep-alive eggjs/egg#4527

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: provide keep-alive timeout response header #34561

http: provide keep-alive timeout response header #34561

ronag commented Jul 30, 2020

ronag commented Jul 30, 2020

ronag Jul 30, 2020

ronag Jul 30, 2020

ronag commented Jul 30, 2020

nodejs-github-bot commented Jul 30, 2020

nodejs-github-bot commented Jul 30, 2020

mcollina left a comment

ronag commented Aug 1, 2020

killagu Sep 7, 2020

ronag Sep 7, 2020

atian25 Sep 8, 2020

http: provide keep-alive timeout response header #34561

http: provide keep-alive timeout response header #34561

Conversation

ronag commented Jul 30, 2020

Checklist

ronag commented Jul 30, 2020

ronag Jul 30, 2020

Choose a reason for hiding this comment

ronag Jul 30, 2020

Choose a reason for hiding this comment

ronag commented Jul 30, 2020

nodejs-github-bot commented Jul 30, 2020

nodejs-github-bot commented Jul 30, 2020

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Aug 1, 2020

killagu Sep 7, 2020

Choose a reason for hiding this comment

ronag Sep 7, 2020

Choose a reason for hiding this comment

atian25 Sep 8, 2020

Choose a reason for hiding this comment